Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: check if headersSent #845

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

Conversation

Leechael
Copy link

Description

Check response.headersSent in both handleSuccess & handleError, prevents that response already sent and Express app crashed.

@attilaorosz
Copy link
Member

While I agree with this, I feel like we should somehow indicate that your response from the controller will not be delivered. Simply refusing to return your response from the controller might cause a lot of confusion and debugging. @NoNameProvided thoughts?

@Leechael
Copy link
Author

In my case, I'm create a middleware prevents long-run response (for example, if the API not response in 5 seconds, just send 504 then don't delivery the long-run response).

Without the check the handleSuccess / handleError will failed and crashed.

@attilaorosz
Copy link
Member

I get your point and I agree with it, my only concern is that it might cause unexpected results. We should somehow indicate (log?) that this happened. For example just adding and error.log letting the dev know that the controller result is ignored because the headers are already sent. Obviously console.log is not the perfect solution, just a low hanging fruit :)

@Leechael
Copy link
Author

I'm agree to improve that, how about optional warning to console.log? and the warning and opt-out.

@attilaorosz
Copy link
Member

Lets go with a warning now if the headers are already sent.

@attilaorosz
Copy link
Member

Could you please add a tests?

@Leechael
Copy link
Author

Could you please add a tests?

Sure. Let me check how to do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants